I think true might be a more natural default value for this?

Changed to true. The false default value was to make the tests in "ranking across signals" and "final results end-to-end" of "ChannelLinkAutocompleteView" group a little less verbose.🙂

channel: Add isRecentlyActive field

Since we're already not matching the order in the API doc (see e.g. #1894 (comment) ), I'd put this next to the related-looking field streamWeeklyTraffic, perhaps just above it without an empty line in between.

Similarly elsewhere in this commit.

We try to reserve assert for invariants that are our own responsibility, i.e. those that won't be broken except for some broken logic in the client. Here, the invariant exists, but it's one that can be broken by something out of our control, in particular a badly-behaving server.

Also, asserts don't run in production, so this won't work as "crunchy shell" validation. It makes sense to want such validation, so the "soft center" of the app can rely on this invariant. But let's do it in ChannelDeleteEvent.fromJson; for an example to follow, see DeleteMessageEvent.fromJson.

nit: we'd normally say TODO(server-10) make required or just TODO(server-10).

Oh actually, we can make this nicer by encapsulating this conditional in the API-binding layer—ChannelDeleteEvent can have just final List<int> streamIds (maybe channelIds, as the more modern name?), and it can read its value depending on what the JSON looks like.

Something like this (untested)?:

/// A [ChannelEvent] with op `delete`: https://zulip.com/api/get-events#stream-delete
@JsonSerializable(fieldRename: FieldRename.snake)
class ChannelDeleteEvent extends ChannelEvent {
  @override
  @JsonKey(includeToJson: true)
  String get op => 'delete';

  @JsonKey(readValue: _readChannelIds)
  final List<int> channelIds;

  // TODO(server-10) simplify away; rely on stream_ids
  static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
    final channelIds = json['stream_ids'] as List<int>?;
    if (channelIds != null) return channelIds;

    final channels = json['streams'] as List<dynamic>;
    return channels
      .map((c) => (c as Map<String, dynamic>)['stream_id'] as int)
      .toList();
  }

  ChannelDeleteEvent({
    required super.id,
    required this.channelIds,
  });

  factory ChannelDeleteEvent.fromJson(Map<String, dynamic> json) =>
    _$ChannelDeleteEventFromJson(json);

  @override
  Map<String, dynamic> toJson() => _$ChannelDeleteEventToJson(this);
}

(In that code, the crunchy-shell validation is done by the final channels = json['streams'] as List<dynamic>; line, which will throw if both .stream_ids and .streams are absent in the json.)

Changed to the new version. One thing that this does is that in toJson, there will be an entry with key channel_ids; not exactly what the server gives us (stream_ids or streams). Should we edit the toJson method to match the server data, or is it not that important since we don't use it to send it back to the server?

Ahh, so there was a test failing in test/model/store_test.dart and the fix was to include streams in toJson.

Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.

I took a look and found a bug in _readChannelIds in this revision 🙂:

diff --git lib/api/model/events.dart lib/api/model/events.dart
index 6a0d9ffa4..4d9c5121c 100644
--- lib/api/model/events.dart
+++ lib/api/model/events.dart
@@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent {
   // TODO(server-10) simplify away; rely on stream_ids
   static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
     final channelIds = json['stream_ids'] as List<dynamic>?;
-    if (channelIds != null) channelIds.map((id) => id as int).toList();
+    if (channelIds != null) return channelIds.map((id) => id as int).toList();
 
     final channels = json['streams'] as List<dynamic>;
     return channels

Neat! Unfortunately we're not ready to show the rendered channel description; some of our content widgets will break if they appear outside the context of a message, because they use InheritedMessage.of(context), and we need to address that systematically, which is #488. See related issues:

• Show channel description in channel action sheet #1896
• content: Support Zulip content outside messages (even outside per-account contexts) #488

For now let's do as I did in #1877:

• Not try to show the channel description here
• File an issue for it and leave a TODO

Filed #1945. Also, looking at https://github.com/zulip/zulip-flutter/blob/main/lib/widgets/content.dart, it seems like InheritedMessage.of(context) is used in two places, MessageImagePreview and MessageInlineVideo, and by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now. But as it’s possible that InheritedMessage.of(context) will be used in other widgets, it's good to wait for #488 as you mentioned.

by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now.

For this sort of detail I'd want to rely on API guarantees rather than manual testing on a current server. It's at least plausible that we could run into different behavior with some old server we still support (like 7), or even on any server, including a modern one like CZO, for a channel that was last updated when the server version was ancient, like 3 or something.

But as it’s possible that InheritedMessage.of(context) will be used in other widgets, it's good to wait for #488 as you mentioned.

Yep, this is also true :) it'll be nice to be systematic about it.

Am I reading web's sort_streams correctly that it also considers channel descriptions in the filtering and ranking? I don't personally think we need to do that, but it probably deserves a mention here.

I'd prefer not to add and use tryCast for this, and instead do something like:

which is equivalent and doesn't add a step for the reader to interpret where null comes from and what it means. It also separates the main, headline logic from the rest, corresponding to the dartdoc's choice of what goes in its first line vs. the body:

  /// Comparator that puts subscribed channels before unsubscribed ones.
  ///
  /// For subscribed channels, it puts them in the following way:
  ///   pinned unmuted > unpinned unmuted > pinned muted > unpinned muted

(also nit: s/way/order/ in that dartdoc)

This is a very reasonable definition of "weekly traffic" 🙂 and so isn't likely to bitrot i.e. become incorrect over time. But since we're just using ZulipStream.streamWeeklyTraffic directly (no computations on it), let's leave that field's definition as the single place where we write its definition, so we only have to change that one thing if the meaning changes.

…I see that we haven't actually written down the field's meaning, which we might've done in dartdoc on the field. But that's quite normal and appropriate; by leaving it blank, we mean to defer to the API documentation, which is linked in the class ZulipStream dartdoc.

This should be awaited; similarly in a few other places.

Thanks to the discarded_futures lint for catching this, actually; I was playing with it for #731 this morning 🙂

nit: bump on #1902 (comment)

nit: bump on #1902 (comment)

Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.

I took a look and found a bug in _readChannelIds in this revision 🙂:

diff --git lib/api/model/events.dart lib/api/model/events.dart
index 6a0d9ffa4..4d9c5121c 100644
--- lib/api/model/events.dart
+++ lib/api/model/events.dart
@@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent {
   // TODO(server-10) simplify away; rely on stream_ids
   static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
     final channelIds = json['stream_ids'] as List<dynamic>?;
-    if (channelIds != null) channelIds.map((id) => id as int).toList();
+    if (channelIds != null) return channelIds.map((id) => id as int).toList();
 
     final channels = json['streams'] as List<dynamic>;
     return channels

Sure! Sounds like that would help us avoid a bug like the one fixed here.

Also could you add test cases for the strings "#124"

Ah! It seems GitHub mangled my comment: I wrote zulip/zulip-flutter#124, but it linkified it the same way as it linkifies #124, because this is a PR in zulip/zulip-flutter.

Interesting. Looks like web also does the corresponding thing for @-mentions: https://github.com/zulip/zulip/blob/1e78447c5/web/src/composebox_typeahead.ts#L516-L531

function filter_mention_name(current_token: string): string | undefined {
    if (current_token.startsWith("**")) {
        current_token = current_token.slice(2);
    } else if (current_token.startsWith("*")) {
        current_token = current_token.slice(1);
    }
    if (current_token.includes("*")) {
        return undefined;
    }

    // Don't autocomplete if there is a space following an '@'
    if (current_token.startsWith(" ")) {
        return undefined;
    }
    return current_token;
}

This is maybe more important for channel/topic autocomplete, right? Because (once the topic part is implemented) you might backspace as part of figuring out how to get just a channel link without a topic. But this is a good prompt to file an issue and add a TODO for doing this with @-mention autocomplete; would you do those?

Sure, that would be an improvement. I have always missed that feature for @-mentions; filed #1967.

I think we normally show "(unknown channel)" in italics, to distinguish it from a potential channel with that name.

The rows have 44px height in the Figma, but this gives 32px height (unless increased via the device text-size setting).

To bring it up to 44px, we could structure it similarly to MentionAutocompleteItem—

—which could be helpful in a potential future where we made a generic AutocompleteItem widget that serves both kinds of autocomplete.

Just channelLink, I think; the "syntax" part is implied. (As in existing functions in this file, like quoteAndReply which creates quote-and-reply syntax, or userMention which creates user-mention syntax.)

channel: Finish channel link autocomplete for compose box

Could you separate this special character-replacement logic into its own commit? I want to see if we can ground our reasoning in API documentation. As far as that goes, there's nothing "invalid" or "faulty" about these characters appearing in channel names.

This should be just bool, right? This property will be a bool when it exists at all.

It's kind of confusing how this alternates between channelId and channel.streamId. Those should be equal. It'd therefore be clearer to assert those are equal, and then use channelId throughout.

Similarly, since this now looks up streams[channelId] up front, it can skip repeating that lookup in these asserts.

Rather than look up the channel by ID once, then look it up again as part of remove, this can call remove up front and keep the return value.

(The existing code has asserts that lack that kind of optimization — but they're asserts, so only run in debug builds anyway and optimization is less of a concern.)

nit: I think a switch statement (like in MentionAutocompleteView) would be easier to read here.

One reason is that the formatting would be more consistent that way:

• each case gets one line (vs. here where TopicNarrow appears off on the right);
• each outcome starts at the same column (vs. here where streamId, null, and the IIFE all start in quite different places).

The switch statement would also simplify diffs when adding (or removing) subclasses for Narrow: just insert (or remove) lines case FooNarrow:, vs. here where existing lines might need to be edited to add or remove a || prefix or => () { suffix. Put another way: it's helpful when the different items in a list are all formatted the same, vs. the first or last items being formatted differently.

Sure, make sense.

Is that behavior desired?

From the code at your handy link above (https://github.com/zulip/zulip/blob/c3fdee6ed/web/src/typeahead_helper.ts#L972-L988), it looks like web treats unknown weekly traffic equal to zero.

From the API docs for stream_weekly_traffic:

If null, the channel was recently created and there is insufficient data to estimate the average traffic.

That seems like it should rank at least as high as a value of zero.

So probably we should just match the web behavior.

When both channels match, this gives -1 but should give 0.

The corresponding logic in web seems to be a bit more complicated, in stream_list_sort.ts:

export function has_recent_activity(sub: StreamSubscription): boolean {
    if (!filter_out_inactives || sub.pin_to_top) {
        // If users don't want to filter inactive streams
        // to the bottom, we respect that setting and don't
        // treat any streams as dormant.
        //
        // Currently this setting is automatically determined
        // by the number of streams.  See the callers
        // to set_filter_out_inactives.
        return true;
    }
    return sub.is_recently_active || sub.newly_subscribed;

Can you explain why we don't match those other wrinkles?

The main reason for not including other criteria was that we don't have all the information. For example, sub.newly_subscribed. We already favor pinned channels (sub.pin_to_top). And for filter_out_inactives, I think we can know about it by getting the value for demote_inactive_streams in UserSettings, but I thought it would be simpler to ingore it for now. But if we want to use it, I'll be glad to add it in the next revision. 🙂

(Added some comments that briefly mention this difference.)

No need to add those in this PR 🙂.

I'd like to have the complete list of differences in a comment in the code, though — or at least the full list of what you're aware of.